Skip to content

Conversation

ruuda
Copy link
Contributor

@ruuda ruuda commented Aug 3, 2017

This came up in #43631: there can be long relative urls in Markdown comments, that do not start with http:// or https://, so the tidy check will not detect them as urls and complain about the line length. This PR adds detection of relative urls starting with ../.

@rust-highfive
Copy link
Contributor

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@Mark-Simulacrum
Copy link
Member

Code looks good to me. @rust-lang/docs, can you think of any accidental ignores we'd add by doing this? I'd imagine this is fine...

@GuillaumeGomez
Copy link
Member

It seems good for me.

@@ -79,11 +79,11 @@ fn line_is_url(line: &str) -> bool {
=> state = EXP_URL,

(EXP_LINK_LABEL_OR_URL, w)
if w.starts_with("http://") || w.starts_with("https://")
if w.starts_with("http://") || w.starts_with("https://") || w.starts_with("../")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so this change will permit long lines anywhere that there is a word beginning with ../. I'm not sure if this is what we want. Maybe it is, or maybe it's too accepting.

On the other hand, if we only keep the second change, below, then the [deref-coercions]: link in the relevant PR would put us into the EXP_URL state, and there we can accept ../, where the context is a bit less ambiguous.

I could go either way, just saying that maybe we don't want this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is currently one line in the codebase which would be affected:

$ git grep -E '\s?//(/|!)?\s\.\./'
src/libstd/net/tcp.rs:    /// ../../std/net/trait.ToSocketAddrs.html#tymethod.to_socket_addrs

It is part of a Markdown link.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in favour of removing (though I think it's just the first word on a line, rather than any).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

Yellow is indeed a nice color for a bikeshed.
@shepmaster shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 4, 2017
@arielb1
Copy link
Contributor

arielb1 commented Aug 8, 2017

r? @Mark-Simulacrum - @nikomatsakis is on vacation until Aug 15

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Aug 9, 2017

📌 Commit 608863d has been approved by Mark-Simulacrum

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 11, 2017
…ark-Simulacrum

Detect relative urls in tidy check

This came up in rust-lang#43631: there can be long relative urls in Markdown comments, that do not start with `http://` or `https://`, so the tidy check will not detect them as urls and complain about the line length. This PR adds detection of relative urls starting with `../`.
bors added a commit that referenced this pull request Aug 11, 2017
MaloJaffre added a commit to MaloJaffre/rust that referenced this pull request Aug 11, 2017
…ark-Simulacrum

Detect relative urls in tidy check

This came up in rust-lang#43631: there can be long relative urls in Markdown comments, that do not start with `http://` or `https://`, so the tidy check will not detect them as urls and complain about the line length. This PR adds detection of relative urls starting with `../`.
@bors bors merged commit 608863d into rust-lang:master Aug 11, 2017
@ruuda ruuda deleted the allow-long-relative-urls branch August 11, 2017 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants